Enable GPU exection of atm_divergence_damping_3d via OpenACC #1237

abishekg7 · 2024-10-17T21:46:02Z

This PR enables the GPU execution of atm_divergence_damping_3d subroutine.

Tested with a real and idealized test cases.

gdicker1

When I tested this today for the merged branch (develop+atmosphere/port_divergence_damping_3d) versus the base branch (develop), the results matched!

However, I have some comments to address:

gdicker1 · 2024-12-12T21:54:11Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

@@ -2599,6 +2599,13 @@ subroutine atm_divergence_damping_3d( state, diag, mesh, configs, dts, edgeStart
      rdts = 1.0_RKIND / dts
      coef_divdamp = 2.0_RKIND * smdiv * config_len_disp * rdts

+      MPAS_ACC_TIMER_START('atm_divergence_damping_3d [ACC_data_xfer]')
+      !$acc enter data copyin(ru_p, rtheta_pp, rtheta_pp_old, specZoneMaskEdge, &
+      !$acc         theta_m, cellsOnEdge) 


specZoneMaskEdge and cellsOnEdge are invariant fields (the won't change after init). I believe your PR should make sure these variables are handled in atm_dynamics_init and shouldn't copyin or delete these fields in atm_divergence_damping_3d.

Is that the convention we've been following so far? I assumed the routine to be ported has localized data movement clauses. I might also need to rework my other PRs if this is the case. Thanks for pointing it out.

Just to follow up.. Talked to @mgduda, and got this cleared up. Will make changes to these invariant fields. Thanks!

Is that the convention we've been following so far?

I've been following that pattern, it was set in #1176.

I've been doing things similarly to what may happen here - do all the data movement that is needed locally for the function being ported, and then separate out those invariant fields and handle them in the atm_dynamics_{init,finalize} routines. Typically these invariant fields are part of the mesh var_struct as described in the src/core_atmosphere/Registry.xml.

gdicker1 · 2024-12-12T22:05:18Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc         theta_m, cellsOnEdge) 
+      MPAS_ACC_TIMER_STOP('atm_divergence_damping_3d [ACC_data_xfer]')
+
+      !$acc parallel async


async could be dangerous here, I don't believe acc exit data copyout(ru_p) will wait for this kernel to finish before beginning to copy.

I think it would be best to remove this async since we don't need it (yet) and it could create issues for the kernels that come next and depend on ru_p

Will remove the async. thanks!

I removed this async, but wanted to note that there's another one in atm_advance_scalars_work. Is that fine for now?

I removed this async, but wanted to note that there's another one in atm_advance_scalars_work. Is that fine for now?

I think we're good to leave that async clause for now, it has a matching wait directive/clause later on.

abishekg7 · 2024-12-19T23:06:04Z

I've addressed the review comments, and also checked the output restart file for bit-for-bit reproducibility.

gdicker1

With the fixes and my testing¹, this PR has my approval.

Using the regional test case for a few timesteps and comparing the restart, diag, and history output files. The baseline and "feature" branch tests matched! ↩

mgduda · 2025-01-10T22:48:26Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+
+      MPAS_ACC_TIMER_START('atm_divergence_damping_3d [ACC_data_xfer]')
+      !$acc exit data copyout(ru_p) delete(rtheta_pp, rtheta_pp_old, theta_m)
+      !$acc end data


Is this !$acc end data clause needed? (That's an actual, not a rhetorical, question!)

It is needed because of the acc data present, which is currently written as a structured data region. Structured regions in Fortran OpenACC (parallel and data) require an acc end xxx statement.

@gdicker1 Thanks for that explanation! The !$acc data present directive didn't register when I was scanning through the code changes.

@abishekg7 Perhaps it would be better to follow the pattern in other routines that have been ported, and to avoid the use of structured data directives for now?

If we decide to add default(present) clauses to the !$acc parallel directives, that would obviate the need for the !$acc data present(specZoneMaskEdge, cellsOnEdge) directive I would think.

mgduda · 2025-01-10T22:48:42Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc enter data copyin(ru_p, rtheta_pp, rtheta_pp_old, theta_m)
+      MPAS_ACC_TIMER_STOP('atm_divergence_damping_3d [ACC_data_xfer]')
+
+      !$acc parallel


Rather than the !$acc data present directive above on line 2611, it might be simpler and more robust to just add a default(present) clause to this parallel directive.

@gdicker1 Does this align with your understanding of best practices?

I think the best practices would be neutral on this, whatever works best for our code and development. However, I agree that default(present) could be simpler and require less of us.

We would have to ask Pranay or other MPAS v7 OpenACC developers why they have similar function-level acc data present ... directives in their port. Speculation here: I think enclosing the kernels in a structured data region that ensures all data is present prevents the kernels from each checking their data - an optimization concern.

I'm adding the default(present) for now, but noting the potential concern re. performance.

mgduda · 2025-01-10T22:49:24Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc end parallel
+
+      MPAS_ACC_TIMER_START('atm_divergence_damping_3d [ACC_data_xfer]')
+      !$acc exit data copyout(ru_p) delete(rtheta_pp, rtheta_pp_old, theta_m)


For the sake of clarity, I would suggest either having separate directives for copyout and delete, or at least splitting the copyout and delete clauses onto separate lines.

mgduda · 2025-01-15T02:07:21Z

@abishekg7 The code changes look good to me! I'd only suggest reworking the commit history; then I think we'll be all set.

This commit enables the GPU execution of the atm_divergence_damping_3d subroutine using OpenACC directives for the data movements and loops. A new timer, 'atm_divergence_damping_3d [ACC_data_xfer]', has been added for host- GPU data transfers in this subroutine. This port follows these considerations: - The data transfers for the invariant fields have been moved to mpas_atm_dynamics_init and finalize. - Explicitly adding gang, worker and vector level parallelism to parallel constructs in atm_compute_vert_imp_coefs_work. - The parallel constructs use default(present) clauses to avoid implicit data movements by the compiler. This change also requires dereferencing the pointers to nCellsSolve and nVertLevels, in order to avoid runtime error messages such as the following: FATAL ERROR: data in PRESENT clause was not found on device 1:name=ncellssolve

abishekg7 · 2025-01-17T01:47:55Z

Cleaned up the commit history. 1 commit seemed sufficient. Also, tested the rebased version in limited area mode and restart files are still bit identical.

gdicker1 · 2025-01-17T14:35:25Z

@abishekg7 one change, and I think this is good.

The data transfers for the invariant fields have been moved to
mpas_atm_dynamics_init and finalize.

Could you change "have been moved"? It implies such data movement was in the ...damping_3d routine before this commit.

mgduda requested review from mgduda and gdicker1 October 19, 2024 00:26

mgduda added Atmosphere OpenACC Work related to OpenACC acceleration of code labels Oct 19, 2024

gdicker1 suggested changes Dec 12, 2024

View reviewed changes

gdicker1 approved these changes Jan 2, 2025

View reviewed changes

mgduda requested changes Jan 10, 2025

View reviewed changes

mgduda self-requested a review January 15, 2025 02:06

abishekg7 force-pushed the atmosphere/port_divergence_damping_3d branch from 3b82420 to 7db74f8 Compare January 17, 2025 01:46

gdicker1 mentioned this pull request Jan 17, 2025

Enable GPU execution of atm_advance_acoustic_step via OpenACC #1251

Open

mgduda approved these changes Jan 17, 2025

View reviewed changes

mgduda merged commit 37aa961 into MPAS-Dev:develop Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable GPU exection of atm_divergence_damping_3d via OpenACC #1237

Enable GPU exection of atm_divergence_damping_3d via OpenACC #1237

abishekg7 commented Oct 17, 2024

gdicker1 left a comment

gdicker1 Dec 12, 2024

abishekg7 Dec 17, 2024

abishekg7 Dec 18, 2024

gdicker1 Dec 18, 2024

gdicker1 Dec 12, 2024

abishekg7 Dec 17, 2024

abishekg7 Dec 19, 2024

gdicker1 Jan 2, 2025

abishekg7 commented Dec 19, 2024

gdicker1 left a comment

mgduda Jan 10, 2025

gdicker1 Jan 13, 2025

mgduda Jan 14, 2025

mgduda Jan 14, 2025

mgduda Jan 10, 2025

gdicker1 Jan 13, 2025

abishekg7 Jan 15, 2025

mgduda Jan 10, 2025

abishekg7 Jan 15, 2025

mgduda commented Jan 15, 2025

abishekg7 commented Jan 17, 2025

gdicker1 commented Jan 17, 2025

Enable GPU exection of atm_divergence_damping_3d via OpenACC #1237

Enable GPU exection of atm_divergence_damping_3d via OpenACC #1237

Conversation

abishekg7 commented Oct 17, 2024

gdicker1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abishekg7 commented Dec 19, 2024

gdicker1 left a comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgduda commented Jan 15, 2025

abishekg7 commented Jan 17, 2025

gdicker1 commented Jan 17, 2025